fix(router-core): support parsed params in matchRoute#7776
fix(router-core): support parsed params in matchRoute#7776LadyBluenotes wants to merge 19 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTyped route matching
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant useMatchRoute
participant RouterCoreMatchRoute
participant RouteTreeMatcher
participant RouteParamParser
useMatchRoute->>RouterCoreMatchRoute: Match route with typed params
RouterCoreMatchRoute->>RouteTreeMatcher: Resolve destination branch
RouteTreeMatcher->>RouteParamParser: Parse extracted parameters
RouteParamParser-->>RouteTreeMatcher: Return parsed params or rejection
RouteTreeMatcher-->>RouterCoreMatchRoute: Return parsed route match
RouterCoreMatchRoute-->>useMatchRoute: Return typed match or false
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit c1f3318
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
…ck/router into match-route-parsed-params
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths redirect (solid) |
440.8 KB | 922.3 KB | -52.21% |
| ❌ | Simulation | ssr before-load chain (react) |
50 ms | 55 ms | -9.03% |
| ❌ | Simulation | client-route-tree-scale navigation loop (react) |
74.1 ms | 77.2 ms | -3.99% |
| ❌ | Simulation | ssr streaming deferred (react) |
70.6 ms | 72.9 ms | -3.19% |
| ⚡ | Memory | mem server error-paths unmatched (react) |
606.9 KB | 264.9 KB | ×2.3 |
| ⚡ | Memory | mem server streaming-peak chunked (vue) |
13.9 MB | 11.2 MB | +23.75% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
274.7 KB | 263.2 KB | +4.34% |
| ⚡ | Memory | mem server server-fn-churn (react) |
283 KB | 272.3 KB | +3.94% |
| ⚡ | Simulation | client-async-pipeline navigation loop (react) |
51.7 ms | 50.1 ms | +3.28% |
| ⚡ | Memory | mem server error-paths redirect (vue) |
304.6 KB | 295.3 KB | +3.18% |
| ⚡ | Memory | mem server error-paths not-found (solid) |
427 KB | 414.2 KB | +3.09% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing match-route-parsed-params (9276fdd) with main (a3e24c3)
Sheraff
left a comment
There was a problem hiding this comment.
I think it makes sense to fix this. But also router.matchRoute is broken more generally because it does not take the route tree into account. It could return true for the specific route you're asking for, but when actually compared against the entire tree, another route would match because it had higher matching priority.
Added a regression for |
|
@Sheraff made the changes we talked about in discord |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router-core/src/new-process-route-tree.ts (2)
812-849: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
findMatchalways redoes full extraction from scratch, discarding the incrementalextractstate already computed duringgetNodeMatch.When
includeRouteParamsis set, the constructedleafobject omitsextract(onlynode,skipped,rawParamsare carried over), forcingextractParamsto restart frompartIndex/nodeIndex/pathIndex/segmentCount = 0even though the winning frame may already have partially-resumedextractstate from earliervalidateParseParamscalls along the branch. This is correct but redundant for branches that use.parse(), doing O(n) extraction twice. Worth a look if this path is hot, though likely low priority given branch depth is typically small.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/new-process-route-tree.ts` around lines 812 - 849, Reuse the incremental extract state from getNodeMatch in findMatch instead of restarting extractParams from zero. Preserve the winning leaf frame’s extract data when constructing or passing the leaf, and update extractParams to continue from that state while retaining existing rawParams and matchData behavior.
688-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
RouteMatchDatais defined but not reused for the identical inline shapes.The new
RouteMatchDatatype (line 858-862) exactly matches thematchDataelement shape that's independently inlined three more times:RouteMatch<T>.matchData(692-696),findMatch's return type (820-824), andextractParams's return tuple (885-889). Consolidating onto the shared alias avoids the four copies drifting apart if the shape changes later.♻️ Proposed refactor
type RouteMatch<T extends Extract<RouteLike, { fullPath: string }>> = { route: T rawParams: Record<string, string> branch: ReadonlyArray<T> - matchData: ReadonlyArray<{ - rawParams?: Record<string, string> - pathnameEnd: number - caseSensitive: boolean - }> + matchData: ReadonlyArray<RouteMatchData> }): { route: T rawParams: Record<string, string> - matchData?: ReadonlyArray<{ - rawParams?: Record<string, string> - pathnameEnd: number - caseSensitive: boolean - }> + matchData?: ReadonlyArray<RouteMatchData> } | null {extractedParams: Record<string, string>, - matchData?: Array<{ - rawParams?: Record<string, string> - pathnameEnd: number - caseSensitive: boolean - }>, + matchData?: Array<RouteMatchData>, ] {(Note: TypeScript type aliases aren't order-sensitive, so
RouteMatchDatacan be referenced ahead of its declaration without issue.)Also applies to: 813-825, 858-863, 885-889
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/new-process-route-tree.ts` around lines 688 - 696, Reuse the shared RouteMatchData alias for every matchData element type instead of repeating its inline shape. Update RouteMatch<T>.matchData, findMatch’s return type, and extractParams’ return tuple to reference RouteMatchData, while keeping the alias definition and existing type structure unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Line 951: Update the one-line conditionals in the route-tree logic: wrap the
body of the `if (part)` statement around `pathIndex += part.length` and the body
of the `if (result === false)` statement around `return null` in curly braces,
preserving their existing behavior.
---
Nitpick comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Around line 812-849: Reuse the incremental extract state from getNodeMatch in
findMatch instead of restarting extractParams from zero. Preserve the winning
leaf frame’s extract data when constructing or passing the leaf, and update
extractParams to continue from that state while retaining existing rawParams and
matchData behavior.
- Around line 688-696: Reuse the shared RouteMatchData alias for every matchData
element type instead of repeating its inline shape. Update
RouteMatch<T>.matchData, findMatch’s return type, and extractParams’ return
tuple to reference RouteMatchData, while keeping the alias definition and
existing type structure unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af4b87d9-0cd5-46e6-9821-c5369b926b71
📒 Files selected for processing (7)
packages/router-core/src/new-process-route-tree.tspackages/router-core/src/router.tspackages/router-core/tests/match-by-path.test.tspackages/router-core/tests/match-route.test.tspackages/router-core/tests/optional-path-params-clean.test.tspackages/router-core/tests/optional-path-params.test.tspackages/router-core/tests/path.test.ts
💤 Files with no reviewable changes (1)
- packages/router-core/tests/match-by-path.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/router.ts
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this build failure as an environment issue rather than a code change. The failing project tanstack-start-example-rscs is not touched by this PR, and the error (ERR_PACKAGE_PATH_NOT_EXPORTED for nitro's ./runtime/meta subpath) is a pre-existing dependency incompatibility between nitro-nightly and the installed nitro version. Our changes to route matching logic have no bearing on this failure.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
matchRoutenow resolves matches through the processed route tree.params.parseresults before comparing or returning params.findSingleMatchand its cache withfindRouteMatch.caseSensitive, basepath, search, pending, optional-param, and wildcard behavior covered.matchRoutenow returnsfalsefor destination templates that are not registered in the route tree.Includes the reproducer from #2460.
Closes #2450.
Summary by CodeRabbit
params.parse/params.stringify, including proper numeric conversions.matchRoutenow returns parsed parameters (not raw strings) and correctly fails when typed parsing is rejected or throws.searchand trailing-slash behavior.matchRouteanduseMatchRoutetest coverage for React, Solid, and Vue, covering typed params, fuzzy matching, and mismatch cases.